-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
usbus: Add support for endpoint halt condition #17090
Conversation
1560c3f
to
6574ca3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicks
sys/include/usb/descriptor.h
Outdated
#define USB_FEATURE_DEVICE_REMOTE_WAKEUP 0x01 /**< Device remote wakeup */ | ||
#define USB_FEATURE_ENDPOINT_HALT 0x00 /**< Endpoint halt */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define USB_FEATURE_DEVICE_REMOTE_WAKEUP 0x01 /**< Device remote wakeup */ | |
#define USB_FEATURE_ENDPOINT_HALT 0x00 /**< Endpoint halt */ | |
#define USB_FEATURE_ENDPOINT_HALT 0x00 /**< Endpoint halt */ | |
#define USB_FEATURE_DEVICE_REMOTE_WAKEUP 0x01 /**< Device remote wakeup */ |
sys/include/usb/usbus.h
Outdated
* @brief Clear the halt condition on an endpoint | ||
* | ||
* @note Must only be used when the endpoint is halted and when the host issues | ||
* a SetInterface request on the interface containing the endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* a SetInterface request on the interface containing the endpoints | |
* a SetInterface request on the interface containing the endpoint |
@bergzand I tried on SAME54-XPRO with your patch applied on top on this branch.
But after that, I cannot ping anymore the board. Looks like the board didn't recover. |
The overall problem with this PR is that the handler (CDC-ECM in this case) is not aware that the halt condition got resolved and that it can restart the pipe to the host. I think this needs some callback (not sure if we can reuse one) triggered at the end of the clear halt function. On the CDC-ECM end this would flush the RX status. |
This truncates the incomming frames to ETHERNET_FRAME_LEN and silently discards the rest of the frame until the end of the frame. This should be modified to an endpoint halt condition after RIOT-OS#17090 is merged, but for now this should be good enough. Stalling the endpoint with the current stall implementation could cause a ping of death scenario, so for now the data is truncated until the above solution can be implemented.
What's the state here? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
Please rebase. |
6574ca3
to
5fd5618
Compare
rebased! |
Main issue with this PR so far is that there is no mechanism to communicate the halt clear back to the interface. Meaning that in the test example above, the CDC ECM interface is not aware that communication can resume. This needs an extension of the USBUS interface api |
Instead of directly stalling an endpoint, handlers should enable the halt condition on an usbus endpoint to signal error condition. This can then be cleared via a CLEAR FEATURE request from the host.
This extends support for the GET STATUS requests to support endpoints and interfaces as recipient. It also adds the SET and CLEAR FEATURE requests for the endpoints with support to set and clear the halt condition on an endpoint.
aa3b6c0
to
7b2db7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it requires some API changes to also inform the interfaces when the EP status changes on a CLEAR FEATURE request, we should merge it for now.
bors merge |
17090: usbus: Add support for endpoint halt condition r=gschorcht a=bergzand ### Contribution description This PR adds support for the endpoint halt condition on the USBUS side. Instead of directly stalling an endpoint, handlers should enable the halt condition on an usbus endpoint to signal error condition. This can then be cleared via a CLEAR FEATURE request from the host. This PR also extends support for the GET STATUS requests to support endpoints and interfaces as recipient. It also adds the SET and CLEAR FEATURE requests for the endpoints with support to set and clear the halt condition on an endpoint. ### Testing procedure The feature can be shown when adding the following patch to the CDC_ECM code: ```Patch diff --git a/sys/usb/usbus/cdc/ecm/cdc_ecm.c b/sys/usb/usbus/cdc/ecm/cdc_ecm.c index f580209e03..3d87a67087 100644 --- a/sys/usb/usbus/cdc/ecm/cdc_ecm.c +++ b/sys/usb/usbus/cdc/ecm/cdc_ecm.c `@@` -31,7 +31,7 `@@` #include <string.h> -#define ENABLE_DEBUG 0 +#define ENABLE_DEBUG 1 #include "debug.h" static void _event_handler(usbus_t *usbus, usbus_handler_t *handler, `@@` -335,7 +335,12 `@@` static void _transfer_handler(usbus_t *usbus, usbus_handler_t *handler, size_t len = 0; usbdev_ep_get(ep, USBOPT_EP_AVAILABLE, &len, sizeof(size_t)); _store_frame_chunk(cdcecm); - if (len == USBUS_CDCECM_EP_DATA_SIZE) { + DEBUG("Length: %u\n", cdcecm->len); + if (cdcecm->len > 1000) { + usbus_endpoint_halt(cdcecm->ep_out); + _handle_rx_flush(cdcecm); + } + else if (len == USBUS_CDCECM_EP_DATA_SIZE) { usbdev_ep_ready(ep, 0); } } diff --git a/sys/usb/usbus/usbus.c b/sys/usb/usbus/usbus.c index 826966c929..1bed707e2d 100644 --- a/sys/usb/usbus/usbus.c +++ b/sys/usb/usbus/usbus.c `@@` -40,7 +40,7 `@@` #include <string.h> #include <errno.h> -#define ENABLE_DEBUG 0 +#define ENABLE_DEBUG 1 #include "debug.h" #define _USBUS_MSG_QUEUE_SIZE (16) ``` With this patch, the CDC ECM code will trigger the halt condition after receiving more than 1000 bytes for a single network packet. For example with a `ping 2001:db8::2 -s 1200` (assuming `2001:db8::2` is the RIOT device). On a nRF52840dk this looks like the following: <details><summary>nRF52840dk</summary> ``` 2021-10-31 21:24:26,271 # usbus: starting thread 3 2021-10-31 21:24:26,275 # usbus: Adding string descriptor number 1 for: "USB config" 2021-10-31 21:24:26,281 # usbus: Adding string descriptor number 2 for: "USB device" 2021-10-31 21:24:26,286 # usbus: Adding string descriptor number 3 for: "RIOT-os.org" 2021-10-31 21:24:26,292 # usbus: Adding string descriptor number 4 for: "AC1B17D1BE432280" 2021-10-31 21:24:26,294 # CDC ECM: initialization 2021-10-31 21:24:26,299 # usbus: Adding string descriptor number 5 for: "AAEC5F69468B" 2021-10-31 21:24:26,305 # NETOPT_RX_END_IRQ not implemented byusbus: USB suspend detected 2021-10-31 21:24:26,312 # main(): This is RIOT! (Version: 2022.01-devel-281-g7da001-pr/usbus/halt_endpoint) 2021-10-31 21:24:26,316 # Test application for the USBUS CDC ECM interface 2021-10-31 21:24:26,317 # 2021-10-31 21:24:26,321 # This test pulls in parts of the GNRC network stack, use the 2021-10-31 21:24:26,327 # provided shell commands (i.e. ifconfig, ping6) to interact with 2021-10-31 21:24:26,330 # the CDC ECM based network interface. 2021-10-31 21:24:26,330 # 2021-10-31 21:24:26,332 # Starting the shell now... > 2021-10-31 21:24:26,481 # CDC ECM: Reset 2021-10-31 21:24:26,545 # CDC ECM: Reset 2021-10-31 21:24:26,631 # CDC ECM: Request: 0xb 2021-10-31 21:24:26,635 # CDC ECM: Changing active interface to alt 1 2021-10-31 21:24:26,638 # CDC ECM: sending link up indication 2021-10-31 21:24:26,645 # CDC ECM: Request: 0x43 2021-10-31 21:24:26,649 # CDC ECM: Not modifying filter to 0xc 2021-10-31 21:24:29,889 # CDC ECM: Request: 0x43 2021-10-31 21:24:29,893 # CDC ECM: Not modifying filter to 0xc 2021-10-31 21:24:29,901 # CDC ECM: Request: 0x43 2021-10-31 21:24:29,904 # CDC ECM: Not modifying filter to 0xe 2021-10-31 21:24:29,907 # CDC ECM: sending link speed indication > ifconfig 4 add 2001:db8::2/64 2021-10-31 21:24:35,296 # ifconfig 4 add 2001:db8::2/64 2021-10-31 21:24:35,300 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:24:35,305 # success: added 2001:db8::2/64 to interface 4 2021-10-31 21:24:35,307 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:24:51,075 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:24:51,078 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:06,851 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:06,855 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:13,271 # CDC ECM: Request: 0x43 2021-10-31 21:25:13,274 # CDC ECM: Not modifying filter to 0xe 2021-10-31 21:25:13,275 # Length: 64 2021-10-31 21:25:13,276 # Length: 90 2021-10-31 21:25:13,521 # Length: 64 2021-10-31 21:25:13,522 # Length: 86 2021-10-31 21:25:13,569 # Length: 64 2021-10-31 21:25:13,570 # Length: 90 2021-10-31 21:25:14,927 # Length: 64 2021-10-31 21:25:14,927 # Length: 86 2021-10-31 21:25:14,931 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:14,935 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:14,936 # Length: 64 2021-10-31 21:25:14,937 # Length: 128 2021-10-31 21:25:14,938 # Length: 192 2021-10-31 21:25:14,939 # Length: 256 2021-10-31 21:25:14,940 # Length: 320 2021-10-31 21:25:14,941 # Length: 384 2021-10-31 21:25:14,942 # Length: 448 2021-10-31 21:25:14,943 # Length: 512 2021-10-31 21:25:14,945 # Length: 576 2021-10-31 21:25:14,946 # Length: 640 2021-10-31 21:25:14,947 # Length: 704 2021-10-31 21:25:14,948 # Length: 768 2021-10-31 21:25:14,949 # Length: 832 2021-10-31 21:25:14,950 # Length: 896 2021-10-31 21:25:14,951 # Length: 960 2021-10-31 21:25:14,952 # Length: 1024 2021-10-31 21:25:14,954 # Endpoint 1 halted 2021-10-31 21:25:14,955 # Length: 64 2021-10-31 21:25:14,956 # Endpoint 1 unhalted 2021-10-31 21:25:15,961 # Length: 128 2021-10-31 21:25:15,962 # Length: 192 2021-10-31 21:25:15,963 # Length: 256 2021-10-31 21:25:15,964 # Length: 320 2021-10-31 21:25:15,966 # Length: 384 2021-10-31 21:25:15,967 # Length: 448 2021-10-31 21:25:15,968 # Length: 512 2021-10-31 21:25:15,969 # Length: 576 2021-10-31 21:25:15,970 # Length: 640 2021-10-31 21:25:15,971 # Length: 704 2021-10-31 21:25:15,972 # Length: 768 2021-10-31 21:25:15,973 # Length: 832 2021-10-31 21:25:15,974 # Length: 896 2021-10-31 21:25:15,975 # Length: 960 2021-10-31 21:25:15,977 # Length: 1024 2021-10-31 21:25:15,978 # Endpoint 1 halted 2021-10-31 21:25:15,979 # Length: 64 2021-10-31 21:25:15,981 # Endpoint 1 unhalted ``` </details> Of interest in that excerpt are the two "Endpoint 1 halted" and "Endpoint 1 unhalted". In Wireshark, dumping the USB traffic, this looks like: ![image](https://user-images.githubusercontent.com/5160052/139600179-df34d8a2-80b5-4485-a6f6-f3615090126f.png) With the CLEAR FEATURE request visible to clear the stall condition on the endpoint After the stall condition traffic to the device continues again normally without requiring a restart of the device or the USB connection. ### Issues/PRs references Needs #17086 Co-authored-by: Koen Zandberg <koen@bergzand.net>
bors cancel |
Canceled. |
bors merge |
Build succeeded: |
Contribution description
This PR adds support for the endpoint halt condition on the USBUS side.
Instead of directly stalling an endpoint, handlers should enable the
halt condition on an usbus endpoint to signal error condition.
This can then be cleared via a CLEAR FEATURE request from the host.
This PR also extends support for the GET STATUS requests to support endpoints
and interfaces as recipient. It also adds the SET and CLEAR FEATURE
requests for the endpoints with support to set and clear the halt
condition on an endpoint.
Testing procedure
The feature can be shown when adding the following patch to the CDC_ECM code:
With this patch, the CDC ECM code will trigger the halt condition after receiving more than 1000 bytes for a single network packet. For example with a
ping 2001:db8::2 -s 1200
(assuming2001:db8::2
is the RIOT device).On a nRF52840dk this looks like the following:
nRF52840dk
Of interest in that excerpt are the two "Endpoint 1 halted" and "Endpoint 1 unhalted".
In Wireshark, dumping the USB traffic, this looks like:
With the CLEAR FEATURE request visible to clear the stall condition on the endpoint
After the stall condition traffic to the device continues again normally without requiring a restart of the device or the USB connection.
Issues/PRs references
Needs #17086